-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update dependencies #481
Update dependencies #481
Conversation
Huh, more dependencies missing now. Some Always an adventure updating JS dependencies 🤓 |
package.json
Outdated
"@lumino/datagrid": "^0.12.0", | ||
"@lumino/default-theme": "^0.4.4", | ||
"@lumino/widgets": "^1.13.4", | ||
"apollo-boost": "^0.4.9", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we first added Apollo, we used apollo-boost
. Which is just the apollo-client
wrapped by several utility functions, and features like caching.
Later, after realizing we didn't need any of the features in apollo-boost
, I started moving the WorkflowService
to use just ApolloClient
. That also became necessary as we started customizing the client for the WebSockets.
I will have to confirm, but I believe I have finished getting rid of apollo-boost
, but added the dependencies for the non-apollo-boost setting in the devDependencies
.
If so, we can remove the Apollo dependencies from devDependencies
, move them up here, and maybe reduce our list of dependencies, as ApolloBoost includes more code/dependencies.
"cytoscape-expand-collapse": "^3.1.2", | ||
"cytoscape-node-html-label": "^1.1.5", | ||
"cytoscape-expand-collapse": "^4.0.0", | ||
"cytoscape-node-html-label": "^1.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm updating the Graph dependencies, assuming they won't require us to change anything in the Graph component or view 👍 if they demand further work, I will remove these changes to simplify the PR, since we may choose a different library for handling Graphs in Cylc UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to remove the graph view next release, pending @oliver-sanders massive re-do. Probably not good to have a barely-usable POC graph view with wrong look-and-feel alongside our nice (and now performant, post deltas) tree view. (n-distance windows might save graph view performance, but still have the other problems...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least removing dependencies and code is easier. Just need to remember to remove not only cytoscape-
dependencies, but also the tippy
I think. Would be nice if there were dependency groups in NPM, or if at least we could add comments to package.json
(JSON limitation)
|
Now Looks like |
I can now build it with
🤔 |
Codecov Report
@@ Coverage Diff @@
## master #481 +/- ##
==========================================
- Coverage 69.90% 61.43% -8.47%
==========================================
Files 58 42 -16
Lines 1266 918 -348
Branches 78 42 -36
==========================================
- Hits 885 564 -321
+ Misses 361 341 -20
+ Partials 20 13 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Tree view OK. Graph view OK. Mutations view is now broken. Looking into that. From the warning message, it looks like the GraphQL library stopped exporting |
package.json
Outdated
"sinon": "^7.5.0", | ||
"standard": "^14.3.1", | ||
"subscriptions-transport-ws": "^0.9.16", | ||
"lodash": "^4.17.20", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lodash
is actually a runtime dependency. Maybe it's working now if another dependency is loading it (the Apollo libraries do). But this needs to be moved to the dependencies
and removed from the devDependencies
.
The Anyhoo, have modified it to leave the loading to the For tomorrow:
Then it should be ready for review |
Looks good to me. |
PR ready for review. The This branch produces 7.9MB. Most likely related to updates in libraries that have either reduced their code size, or did changes that permit better tree-shaking. Could also be related to the removal of ApolloBoost, replaced by the fine-grained Apollo libraries (ApolloBoost just wraps several libraries into a library for convenience, plus some constructors to get clients with cache, etc). Bruno |
<component | ||
is="VSkeletonLoader" | ||
:ref="widgetId" | ||
:loading="isLoading" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always true if you have nothing in the UI but the mutation. With Graph and Tree views, the subscriptions when started can change the view's isLoading
state.
But the mutation widget doesn't trigger any subscription.
So instead we simply add a Mutation view. The view has a loading state, and displays "Loading..." immediately when loaded. Furthermore, with work to add mutations in the right places, this view might be hidden or removed I guess.
@@ -39,7 +39,7 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. | |||
|
|||
<script> | |||
import gql from 'graphql-tag' | |||
import { introspectionQuery, print } from 'graphql' | |||
import { getIntrospectionQuery as getGraphQLIntrospectionQuery, print } from 'graphql' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in GraphQL 15. Luckily they didn't remove the functionality.
Note that we already have a function in this module called getIntrospectionQuery
, by coincidence. So if we imported like this, we could end up with weird build issues (e.g. recursively calling itself instead of calling the function from the graphql
module). So I've renamed the import 👍
ede7844
to
307a21a
Compare
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in offline mode, graph broken but that might be expected.
I'm getting a lot of "incorrect peer dependency" warnings during installation (though I got some of these before):
warning "@lumino/datagrid > @lumino/[email protected]" has unmet peer dependency "[email protected]".
warning "apollo-client > [email protected]" has incorrect peer dependency "graphql@^0.11.3 || ^0.12.3 || ^0.13.0 || ^14.0.0".
warning "apollo-link > [email protected]" has incorrect peer dependency "graphql@^0.11.0 || ^0.12.0 || ^0.13.0 || ^14.0.0".
warning " > [email protected]" has unmet peer dependency "prop-types@>=15.5.0".
warning " > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0-0".
warning "@babel/plugin-proposal-class-properties > @babel/[email protected]" has unmet peer dependency "@babel/core@^7.0.0".
warning " > @cypress/[email protected]" has unmet peer dependency "cypress@*".
warning " > @cypress/[email protected]" has unmet peer dependency "@babel/core@^7.0.1".
warning " > @cypress/[email protected]" has unmet peer dependency "@babel/preset-env@^7.0.0".
warning " > @cypress/[email protected]" has unmet peer dependency "babel-loader@^8.0.2".
warning "@vue/cli-plugin-eslint > [email protected]" has incorrect peer dependency "eslint@>=1.6.0 <7.0.0".
warning " > [email protected]" has unmet peer dependency "eslint-plugin-promise@>=4.2.1".
warning " > [email protected]" has incorrect peer dependency "eslint@^6.7.2".
warning " > [email protected]" has incorrect peer dependency "eslint@^5.0.0 || ^6.0.0".
Yup, the graph never worked in offline mode. That's because the mocked WorkflowService only returns the tree data.
Yup, caused by the move to Graphql 15. Most of our dependencies have some ranges like graphql 13 | 14, or something similar to that. But apparently everything that we use is still working. But if you prefer to reduce the risk by moving it down to 14 I can downgrade it too (takes just a few minutes as I'm deleting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, caused by the move to Graphql 15.
Ok, if you've got a handle on what's causing it, looks like the warnings are harmless.
I used `ncu` to get a list of dependencies that had updates available. Then went through these dependencies individually, updating each one and checking if the build was working OK. `vuetify` was not updated as it is being updated in cylc#466. Updating `eslint-config-vuetify` resulted in some weird errors in `yarn install`: error An unexpected error occurred: "could not find a copy of eslint to link in /home/kinow/Development/python/workspace/cylc-ui/node_modules/eslint-config-vuetify/node_modules". So I reverted it and will take care of that dependency later (it is a dev dependency).
…d after updating dependencies.
See more on their issue at miaolz123/vue-markdown#18
…so is Axios (user-profile service)
307a21a
to
04d701a
Compare
Rebased |
This is a small change with no associated Issue.
I used
ncu
to get a list of dependencies that had updates available. Then went through these dependencies individually, updating each one and checking if the build was working OK.vuetify
was not updated as it is being updated in #466.Updating
eslint-config-vuetify
resulted in some weird errors inyarn install
:So I reverted it and will take care of that dependency later (it is a dev dependency).
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.